Correct Color for RichText links in Dark Mode #2404

chipsnyder · 2020-06-18T21:56:29Z

Fixes #2399
Related PRs:
gutenberg WordPress/gutenberg#23289
AztecEditor-iOS wordpress-mobile/AztecEditor-iOS#1295
~~WordPress-Android wordpress-mobile/WordPress-Android#12226~~
~~WordPress-iOS wordpress-mobile/WordPress-iOS#14351~~

Solution Description

As it turns out this error seems to be one that will randomly happen. Android uses the following process to set the text and style on RichText components:

The text is set
This then updates the text on AztexText
The links are then styled as part of that call
Which is then dependent on linkFormatter to determine the span and set the style This needs both the text and for linkFormatter to be set correctly.

For the link color

When the linkTextColor is set this updates linkFormatter with the proper attributes.

The Issue
It appears that the @ReactProp's don't enforce an order of operations[1]. So if linkTextColor is processed before text everything will work fine. If not then the app will fall back to the default styles.

The Fix
To fix this I simply moved the linkTextColor to be part of the text attribute that is set so that we can enforce the order of operations.

This is actually only needed for Android however to prevent diverging implementations I made the same changes to iOS.

To Test

Set your device too dark mode
Create a post/page or open one that has links.

Color should be using blue-30 (#5198d9)

Before	After

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…he text is formatted

…eLinks

…ch the pattern of Android

peril-wordpress-mobile · 2020-06-18T21:56:36Z

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

geriux · 2020-06-22T13:49:39Z

Hey @chipsnyder 👋 thanks for the very well detailed PR!

To fix this I simply moved the linkTextColor to be part of the text attribute that is set so that we can enforce the order of operations.

I know this fixes the issue but I'm not 100% sure about passing it along with the text prop, but of course, we have the issue with React Native not allowing a way to enforce the order. My knowledge of Aztec is very limited so I was wondering what @mchowning thinks about it. Matt, whenever you have time would you mind checking this approach, please? I just don't wanna overlook something by taking this path 😅

mchowning · 2020-06-22T18:48:00Z

...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java

@@ -202,6 +203,11 @@ public Map getExportedCustomDirectEventTypeConstants() {

    @ReactProp(name = "text")
    public void setText(ReactAztecText view, ReadableMap inputMap) {
+        if (inputMap.hasKey(LINK_TEXT_COLOR_KEY)) {
+            int color = Color.parseColor(inputMap.getString(LINK_TEXT_COLOR_KEY));
+            setLinkTextColor(view, color);


This means that we will be constructing new LinkFormatter and LinkStyle objects on every text change, right? Seems like that is not ideal from a performance perspective, although any impact would probably be negligible. Still, maybe it's worth only updating the link formatter when the link color has changed. I do not feel strongly about this.

Still, maybe it's worth only updating the link formatter when the link color has changed.

So are you thinking something along the lines of this?

Suggested change

setLinkTextColor(view, color);

int color = Color.parseColor(inputMap.getString(LINK_TEXT_COLOR_KEY));

if (view.linkFormatter.color != color) {

setLinkTextColor(view, color);

Note I didn't test this suggestion out yet.

Yes, that's pretty much what I was imagining.

I'm good with that 👍 I'll work on making that change.

mchowning · 2020-06-22T18:48:34Z

...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java

@@ -386,7 +392,6 @@ public void setTextAlign(ReactAztecText view, @Nullable String textAlign) {
        }
    }

-    @ReactProp(name = "linkTextColor", customType = "Color")
    public void setLinkTextColor(ReactAztecText view, @Nullable Integer color) {


Could we make this private now?

We could make this private now. 👍

mchowning · 2020-06-22T18:53:38Z

Matt, whenever you have time would you mind checking this approach, please?

I think this approach makes sense. Seems like a good way to insure a proper order. I noted one small performance-related concern, but I don't think it's a big deal.

An alternative approach that might work would be to reset the text any time the linkTextColor prop comes down, but I wouldn't be surprised if that had some unwanted side effects.

I haven't built this and tested it out, but looking at the code it looks good to me. 👍

Hey @chipsnyder 👋 thanks for the very well detailed PR!

+1000 to that! 🙂

chipsnyder · 2020-06-22T19:20:15Z

An alternative approach that might work would be to reset the text any time the linkTextColor prop comes down, but I wouldn't be surprised if that had some unwanted side effects.

I considered this a bit; however, other items (selection and eventCount) are passed along with the text so I was afraid of the possible side effects.

…eLinks

chipsnyder · 2020-06-25T18:47:54Z

@mchowning @geriux This is good to go again. I made the requested changes and migrated the code to Gutenberg as part of updating it with the monorepo changes.

chipsnyder · 2020-06-25T18:49:00Z

Also, I closed the main App PRs b/c of the monorepo work. So if it's not too much of a hassle can we test with the demo apps?

geriux · 2020-06-29T17:05:04Z

Also, I closed the main App PRs b/c of the monorepo work. So if it's not too much of a hassle can we test with the demo apps?

Hey Chip 👋 The code looks good! I wanted to test on the demo apps but I don't know how to reproduce the bug in develop for example, so then I can switch to this branch and make sure it's fixed. Are there any steps to test a before and after in the demo app? I remember I was able to do it back then but with the main WordPress Android App. Or do you want me to just test that the changes didn't break anything?

Let me know, thanks!

chipsnyder · 2020-06-29T17:22:39Z

Hey @geriux,
Thanks for coming back around to retest this. This might be a bit difficult to reliably recreate due to the nature of the issue being a random order (I'm assuming based on iterating over the keys of a dictionary). I was able to reproduce it in the demo app but I think I just got lucky.

I think for retesting making sure nothing breaks and trying multiple times to refresh the app should help.
Since the Monorepo stuff has merged to the Apps now though I'll go ahead and enable those builds and make new testing builds that way you can at least retest where you know you saw the issue.

WPAndroid: wordpress-mobile/WordPress-Android#12226 (comment)
WPiOS: wordpress-mobile/WordPress-iOS#14351 (comment)

geriux · 2020-06-30T15:36:22Z

Hey @chipsnyder 👋 thanks for bringing back the testing PRs =)

On Android is working great 🎉

On iOS I noticed the colors are different after these changes:

Before	After

This is not expected, right? Let me know, thanks!

chipsnyder · 2020-06-30T16:52:59Z

On iOS I noticed the colors are different after these changes:

Hey @geriux, good catch. Were you running the metro server when you saw this? I totally missed this difference when I checked the builds but it looks like I just failed to rerun the bundle after merging monorepo.

(I'll update the bundle and regenerate the builds)

geriux · 2020-06-30T17:38:14Z

Were you running the metro server when you saw this?

I used the IPA from the WordPress iOS PR without metro.

(I'll update the bundle and regenerate the builds)

Thank you!

chipsnyder · 2020-06-30T18:58:15Z

@geriux those updated builds are available now (Android, iOS) Sorry for that mistake 😅

geriux

Approved via WordPress/gutenberg#23289 (review)

…eLinks

chipsnyder · 2020-07-08T20:55:03Z

Closing this PR as the Gutenberg changes will come with the next release

chipsnyder · 2020-07-08T21:59:55Z

Reopening to keep GB mobile and iOS in sync on the Pod Changes

chipsnyder · 2020-07-08T22:39:29Z

Closing this again. With wordpress-mobile/WordPress-iOS#14444 in place we can just follow the normal release process

chipsnyder added 4 commits June 17, 2020 16:59

Adjust the linkTextColor hook to enforce text colors are set before t…

60fe812

…he text is formatted

Merge remote-tracking branch 'origin/develop' into issue/2399-darkMod…

935caea

…eLinks

Move link text settings to be part of of the text field on iOS to mat…

e93e6e4

…ch the pattern of Android

Update bundle

131811f

chipsnyder added [OS] Android bugfix Dark mode labels Jun 18, 2020

chipsnyder requested a review from geriux June 18, 2020 21:56

This was referenced Jun 18, 2020

[Mobile] Correct Color for RichText links in Dark Mode WordPress/gutenberg#23289

Merged

[Gutenberg] Correct Color for RichText links in Dark Mode - Expose helper method wordpress-mobile/AztecEditor-iOS#1295

Merged

Revert podfile version bump

25d3b60

This was referenced Jun 18, 2020

[Gutenberg] Correct Color for RichText links in Dark Mode - Testing Build wordpress-mobile/WordPress-Android#12226

Closed

[Gutenberg] Correct Color for RichText links in Dark Mode - Testing Build wordpress-mobile/WordPress-iOS#14351

Closed

mchowning reviewed Jun 22, 2020

View reviewed changes

chipsnyder added 2 commits June 24, 2020 16:48

Merge remote-tracking branch 'origin/develop' into issue/2399-darkMod…

3bfb489

…eLinks

Migrate changes to gutenberg for monorepo

ca30cce

Update Release Notes

d6eb4bd

Update Bundle

cac38a7

geriux approved these changes Jul 1, 2020

View reviewed changes

Merge remote-tracking branch 'origin/develop' into issue/2399-darkMod…

c6bdf6a

…eLinks

chipsnyder closed this Jul 8, 2020

chipsnyder deleted the issue/2399-darkModeLinks branch July 8, 2020 20:55

Bump Gutenberg Version

faaa861

chipsnyder reopened this Jul 8, 2020

chipsnyder closed this Jul 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct Color for RichText links in Dark Mode #2404

Correct Color for RichText links in Dark Mode #2404

chipsnyder commented Jun 18, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 18, 2020 •

edited

Loading

geriux commented Jun 22, 2020

mchowning Jun 22, 2020 •

edited

Loading

chipsnyder Jun 22, 2020

mchowning Jun 22, 2020

chipsnyder Jun 22, 2020

mchowning Jun 22, 2020

chipsnyder Jun 22, 2020

mchowning commented Jun 22, 2020

chipsnyder commented Jun 22, 2020

chipsnyder commented Jun 25, 2020

chipsnyder commented Jun 25, 2020

geriux commented Jun 29, 2020

chipsnyder commented Jun 29, 2020 •

edited

Loading

geriux commented Jun 30, 2020

chipsnyder commented Jun 30, 2020 •

edited

Loading

geriux commented Jun 30, 2020

chipsnyder commented Jun 30, 2020

geriux left a comment

chipsnyder commented Jul 8, 2020

chipsnyder commented Jul 8, 2020

chipsnyder commented Jul 8, 2020

Correct Color for RichText links in Dark Mode #2404

Correct Color for RichText links in Dark Mode #2404

Conversation

chipsnyder commented Jun 18, 2020 • edited Loading

Solution Description

To Test

PR submission checklist:

peril-wordpress-mobile bot commented Jun 18, 2020 • edited Loading

geriux commented Jun 22, 2020

mchowning Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

chipsnyder Jun 22, 2020

Choose a reason for hiding this comment

mchowning Jun 22, 2020

Choose a reason for hiding this comment

chipsnyder Jun 22, 2020

Choose a reason for hiding this comment

mchowning Jun 22, 2020

Choose a reason for hiding this comment

chipsnyder Jun 22, 2020

Choose a reason for hiding this comment

mchowning commented Jun 22, 2020

chipsnyder commented Jun 22, 2020

chipsnyder commented Jun 25, 2020

chipsnyder commented Jun 25, 2020

geriux commented Jun 29, 2020

chipsnyder commented Jun 29, 2020 • edited Loading

geriux commented Jun 30, 2020

chipsnyder commented Jun 30, 2020 • edited Loading

geriux commented Jun 30, 2020

chipsnyder commented Jun 30, 2020

geriux left a comment

Choose a reason for hiding this comment

chipsnyder commented Jul 8, 2020

chipsnyder commented Jul 8, 2020

chipsnyder commented Jul 8, 2020

chipsnyder commented Jun 18, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 18, 2020 •

edited

Loading

mchowning Jun 22, 2020 •

edited

Loading

chipsnyder commented Jun 29, 2020 •

edited

Loading

chipsnyder commented Jun 30, 2020 •

edited

Loading